fix(ddns-updater): align template validation standards#657
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Helm validation for ingress hosts and selector label overrides, adjusts ingress host rendering for hostless rules, rewrites NOTES.txt into expanded installation and troubleshooting guidance, and adds tests covering validation and ingress output. ChangesChart validation and documentation update
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Standards Check (GR-079) — PASSEvery changed chart fully passes standards-check. |
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 93.93939% |
✅ Security posture acceptable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
charts/ddns-updater/templates/_helpers.tpl (1)
46-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelector-label keys are duplicated instead of derived from
ddns-updater.selectorLabels.The reserved keys
app.kubernetes.io/name/app.kubernetes.io/instanceare hardcoded here, duplicating the same keys already defined in theddns-updater.selectorLabelstemplate (Lines 29-32). If the selector labels are ever extended, this validation could silently miss new reserved keys.♻️ Suggested refactor: derive reserved keys from the labels template output
{{- define "ddns-updater.validate" -}} {{- if and .Values.ingress.enabled (not .Values.ingress.hosts) -}} {{- fail "ingress.hosts must contain at least one rule when ingress.enabled=true" -}} {{- end -}} +{{- $selectorLabels := fromYaml (include "ddns-updater.selectorLabels" .) -}} {{- range $key, $_ := .Values.podLabels -}} -{{- if or (eq $key "app.kubernetes.io/name") (eq $key "app.kubernetes.io/instance") -}} +{{- if hasKey $selectorLabels $key -}} {{- fail (printf "podLabels must not override selector label %q" $key) -}} {{- end -}} {{- end -}} {{- end -}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/ddns-updater/templates/_helpers.tpl` around lines 46 - 56, The validation in ddns-updater.validate hardcodes reserved pod label keys instead of deriving them from ddns-updater.selectorLabels. Update the podLabels check to reuse the selector label set generated by ddns-updater.selectorLabels so any future selector-label additions are automatically enforced. Keep the existing fail behavior, but replace the duplicated key list with logic that compares against the selector labels template output.charts/ddns-updater/tests/validation_test.yaml (1)
19-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the
app.kubernetes.io/instanceoverride branch.Only the
app.kubernetes.io/nameoverride is tested; the sibling condition forapp.kubernetes.io/instance(in theddns-updater.validatehelper) is untested.✅ Suggested additional test case
- it: should fail when podLabels override selector labels set: podLabels: app.kubernetes.io/name: custom asserts: - failedTemplate: errorMessage: 'podLabels must not override selector label "app.kubernetes.io/name"' + + - it: should fail when podLabels override the instance selector label + set: + podLabels: + app.kubernetes.io/instance: custom + asserts: + - failedTemplate: + errorMessage: 'podLabels must not override selector label "app.kubernetes.io/instance"'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/ddns-updater/tests/validation_test.yaml` around lines 19 - 25, Add a validation test that covers the sibling selector-label override branch in ddns-updater.validate for app.kubernetes.io/instance, since only app.kubernetes.io/name is currently asserted. Mirror the existing failedTemplate pattern in validation_test.yaml by setting podLabels.app.kubernetes.io/instance to a custom value and expecting the corresponding error message that podLabels must not override selector label "app.kubernetes.io/instance".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/ddns-updater/templates/NOTES.txt`:
- Around line 35-41: The pod and log commands in NOTES.txt are using the app
name label only, which can match resources from other releases in the same
namespace. Update the selectors in the note commands to use the release-scoped
label with app.kubernetes.io/instance, matching the pattern already used later
in the file, and apply the same fix to the other referenced note blocks as well.
- Around line 17-22: The NOTES.txt Access guidance currently relies on the with
$host.host block, which suppresses the Web UI URL for hostless or catch-all
ingress setups. Update the ingress notes rendering to keep a generic access
instruction when .Values.ingress.enabled is true but no host is defined, while
still preserving the existing host-specific URL path for named hosts in the same
NOTES.txt template.
---
Nitpick comments:
In `@charts/ddns-updater/templates/_helpers.tpl`:
- Around line 46-56: The validation in ddns-updater.validate hardcodes reserved
pod label keys instead of deriving them from ddns-updater.selectorLabels. Update
the podLabels check to reuse the selector label set generated by
ddns-updater.selectorLabels so any future selector-label additions are
automatically enforced. Keep the existing fail behavior, but replace the
duplicated key list with logic that compares against the selector labels
template output.
In `@charts/ddns-updater/tests/validation_test.yaml`:
- Around line 19-25: Add a validation test that covers the sibling
selector-label override branch in ddns-updater.validate for
app.kubernetes.io/instance, since only app.kubernetes.io/name is currently
asserted. Mirror the existing failedTemplate pattern in validation_test.yaml by
setting podLabels.app.kubernetes.io/instance to a custom value and expecting the
corresponding error message that podLabels must not override selector label
"app.kubernetes.io/instance".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 807c0b89-c82a-42c5-8a72-c5e27a265ea0
📒 Files selected for processing (6)
charts/ddns-updater/templates/NOTES.txtcharts/ddns-updater/templates/_helpers.tplcharts/ddns-updater/templates/ingress.yamlcharts/ddns-updater/templates/validate.yamlcharts/ddns-updater/tests/ingress_test.yamlcharts/ddns-updater/tests/validation_test.yaml
3651393 to
efdf989
Compare
efdf989 to
e84bf07
Compare
|
Reviewed all CodeRabbit feedback in review threads and review-summary comments. Changes:
Validation:
Note: these two CodeRabbit items were posted in the review summary/body, not as active review threads, so there are no thread IDs to reply to or resolve for them. Existing CodeRabbit inline threads are already resolved. |
## Summary - add ddns-updater to the playground chart config mapping after the chart template behavior update ## Validation - npm run lint - npm run format:check - npm run build - make site-sync-check CHART=ddns-updater - make release-check REPO=site - make attribution-check REPO=site Chart PR: helmforgedev/charts#657 Issue: helmforgedev/charts#633 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for the `ddns-updater` chart in the playground, allowing it to appear in the available chart list when applicable. * **Bug Fixes** * Improved chart inclusion behavior so playground results now reflect the configured site sync entries more accurately. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Validation
Site PR: helmforgedev/site#336
Issue: #633
Summary by CodeRabbit
podLabelsfrom overriding chart selector labels.